feat: timeInputV2 #4635
feat: timeInputV2 #4635
Conversation
🦋 Changeset detectedLatest commit: 8d2493c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
lisalupi
requested review from
a team,
BABAK0T0 and
mhervouet
and removed request for
a team
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4635 +/- ##
==========================================
+ Coverage 87.43% 87.76% +0.32%
==========================================
Files 335 340 +5
Lines 12722 13064 +342
Branches 3471 3571 +100
==========================================
+ Hits 11124 11466 +342
Misses 1598 1598
Continue to review full report in Codecov by Sentry.
|
| "@ultraviolet/ui": minor | ||
| --- | ||
|
|
||
| New component `<TimeInputV2 />` |
There was a problem hiding this comment.
Could you implement TimeInputField at same time ? 😅
| @@ -67,6 +67,7 @@ export { TextArea } from './TextArea' | |||
| export { TextInput } from './TextInput' | |||
| export { TextInputV2 } from './TextInputV2' | |||
| export { TimeInput } from './TimeInput' | |||
| export { TimeInputV2, type Time } from './TimeInputV2' | |||
There was a problem hiding this comment.
Not sure what is the purpose of the readonly as you cannot select the content. @mhervouet can you confirm if we need this state and if yes what should be possible to do/
There was a problem hiding this comment.
Ok for readonly you can do same as NextUI
| {timeFormat === 12 ? ( | ||
| <Input | ||
| value={time.period?.toUpperCase()} | ||
| placeholder={placeholder.period ?? '-M'} |
There was a problem hiding this comment.
I'd say we should set AM instead of -M
| } | ||
|
|
||
| return ( | ||
| <Stack key={type} gap={0} direction="row"> |
There was a problem hiding this comment.
| <Stack key={type} gap={0} direction="row"> | |
| <Stack key={type} direction="row"> |
I don't think its needed
| readOnly?: boolean | ||
| error?: boolean | string | ||
| 'data-testid'?: string | ||
| onChange?: (time: Time, timePeriod?: string) => void |
There was a problem hiding this comment.
| onChange?: (time: Time, timePeriod?: string) => void | |
| onChange?: (value: Time, valuePeriod?: string) => void |
To be consistent between our inputs
| data-readonly={readOnly} | ||
| readOnly | ||
| disabled={disabled} | ||
| aria-label={`${fullName()}-input`} |
There was a problem hiding this comment.
Aria label should be set via a prop instead. It should be set when no label is set to give indication to screen reader.
| size?: 'small' | 'medium' | 'large' | ||
| timeFormat?: 12 | 24 | ||
| } |
There was a problem hiding this comment.
here you could add aria-label and conditionally required either label or aria-label
| clearable?: boolean | ||
| required?: boolean | ||
| labelDescription?: ReactNode | ||
| helper?: string |
There was a problem hiding this comment.
Should be a react node, sometimes we need to add a link in the helper
| } | ||
|
|
||
| return ( | ||
| <Stack gap={0.5} className={className} id={id} data-testid={dataTestId}> |
There was a problem hiding this comment.
The id and data-testid should be on the TimeInputWrapper instead
Summary
Type
Summarise concisely:
What is expected?
New component
<TimeInputV2 />